Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

timeutil/timetest: Keep the mutex of timetest.Clock private #64

Merged
merged 1 commit into from
Nov 27, 2023
Merged

timeutil/timetest: Keep the mutex of timetest.Clock private #64

merged 1 commit into from
Nov 27, 2023

Conversation

xavier-petit
Copy link
Contributor

What does this PR do?

I ran grep -rF --include '*.go' .RWMutex. on the repos and found nothing so it should not break anything 🤔

gopls completions before/after hiding the mutex on timetest.Clock

hide

Fixes #

What are the observable changes?

Good PR checklist

  • Title makes sense
  • Is against the correct branch
  • Only addresses one issue
  • Properly assigned
  • Added/updated tests
  • Added/updated documentation
  • Properly labeled

Additional Notes

@xavier-petit xavier-petit self-assigned this Nov 22, 2023
@xavier-petit xavier-petit requested a review from a team as a code owner November 22, 2023 17:26
@xavier-petit xavier-petit requested review from BillotP and pauloestrella1994 and removed request for a team November 22, 2023 17:26
@Sypheos
Copy link
Member

Sypheos commented Nov 22, 2023

That's a bit harder to find out than grep .RWMutex. since the mutex is composed in the Clock. Lock and Unlock can be called directly on the clock. You will need to check for things like cl.Lock etc...
However, there shouldn't be an issue unless someone had the magical idea of passing the clock through an interface to act as some kind of lock.

@xavier-petit
Copy link
Contributor Author

Doh! 🤦🏻 I had the feeling I was missing something.
I used another grep and inspected the few locks in the tests :

grep -rF --files-with-matches --include '*.go' '"github.com/upfluence/pkg/timeutil/timetest"' | xargs grep -E '\.(Lock|Unlock|RWMutex)'

As you said, no one had the magic idea!

@xavier-petit xavier-petit merged commit 7a123ca into master Nov 27, 2023
2 checks passed
@xavier-petit xavier-petit deleted the xp/hide_mutex branch November 27, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sypheos Sypheos Sypheos approved these changes

@tgallice tgallice tgallice approved these changes

@BillotP BillotP Awaiting requested review from BillotP BillotP is a code owner automatically assigned from upfluence/golang

@pauloestrella1994 pauloestrella1994 Awaiting requested review from pauloestrella1994 pauloestrella1994 is a code owner automatically assigned from upfluence/golang

Assignees

@xavier-petit xavier-petit

Labels
reviewed/ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xavier-petit @Sypheos @tgallice @upfluence-bot